Unit tests for get_phase_lag and get_mean_phase_difference
#714
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
===========================================
- Coverage 96.47% 48.59% -47.89%
===========================================
Files 45 45
Lines 9135 9135
===========================================
- Hits 8813 4439 -4374
- Misses 322 4696 +4374 ☔ View full report in Codecov by Sentry. |
|
Note to self and @capy-on-caffeine : this is worth completing :) |
|
Hello @capy-on-caffeine! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-04-22 11:18:43 UTC |
|
@matteobachetti any suggestions for an improved unit test? |
|
Hi @capy-on-caffeine ! One thing that might be worth trying if you want something more fancy is a more realistic case where your lightcurve is a sum of a QPO plus a harmonic (rather than the simple wave you have now), plus the QPO and harmonic have (known) non-zero phase. Then you can test against that known phase rather than 0 (or pi or pi/2) like you are doing now. |
|
@matteolucchini1 Thank you, I'll try this out |
|
@matteobachetti @matteolucchini1 I made some changes, any changes are welcome. |
|
@capy-on-caffeine there is something I don't understand in these tests. Why should phase lags be |
|
Sorry for the late reply. I'll go through these again and change them after discussing. |
| qpo_freq = 0.1 | ||
| harmonic_freq = 2 * qpo_freq | ||
| counts_qpo = 100 * np.sin(2 * np.pi * qpo_freq * time - np.pi / 4) | ||
| counts_harmonic = 50 * np.sin(2 * np.pi * harmonic_freq * time - np.pi / 4) |
There was a problem hiding this comment.
I'm a bit confused by the phases here - it looks like both are -pi/4. Should they not be -pi/4 for one, and +pi/4 for the other, if you want a difference of pi/2?
| qpo_freq = 0.1 | ||
| harmonic_freq = 2 * qpo_freq | ||
| counts_qpo = 100 * np.sin(2 * np.pi * qpo_freq * time + np.pi / 4) | ||
| counts_harmonic = 50 * np.sin(2 * np.pi * harmonic_freq * time + np.pi / 4) |
There was a problem hiding this comment.
Same comment as above - it looks like the phases here are the same. I think I am also misunderstanding something, but should cap_phi_1 and cap_phi_2 not be pi/4 then, and small_psi 0?
|
@matteobachetti @matteolucchini1 I was looking into it and noticed a few things:
|
This PR is for adding unit tests for
get_phase_lagandget_mean_phase_differencein spectroscopy.py as per issue #358. The current tests can be further improved by creating light curves with more data points.